-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nuxt): Instrument Database #17899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: awad/js-1023-nuxtnitro-instrument-cache
Are you sure you want to change the base?
feat(nuxt): Instrument Database #17899
Conversation
7f29d2e
to
55ca147
Compare
c844c9e
to
17482e2
Compare
55ca147
to
76ad329
Compare
size-limit report 📦
|
17482e2
to
65277c7
Compare
68758a1
to
9599b1d
Compare
Co-authored-by: Sigrid Huemer <[email protected]>
65277c7
to
604de80
Compare
7c1e749
to
f9dcb00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks good. Just have some small comments
*/ | ||
export default defineNitroPlugin(() => { | ||
try { | ||
debug.log('@sentry/nuxt: Instrumenting databases...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add conditionals to the debug.log
statements so they are only shown in debug mode. And I think the log already includes [Sentry]
at the start (but not 100%) so we could just simplify it so it shows something like this in the end: [Sentry] [Nitro Database Plugin]: Instrumenting databases...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget the note about conditionally calling it. debug
already handles that
https://github.com/getsentry/sentry-javascript/pull/17858/files#r2418913504
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the messages, and I think we cleared debug.log
being safe to use directly since it does the check on its own.
for (const instance of databaseInstances) { | ||
debug.log('@sentry/nuxt: Instrumenting database instance:', instance); | ||
const db = useDatabase(instance); | ||
instrumentDatabase(db); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this only runs once. But if it doesn't: Maybe we need a conditional here to check whether the database was already instrumented.
604de80
to
f1dd874
Compare
This pull request introduces automatic instrumentation for database queries in Nuxt applications in server side handlers using Sentry.
Implementation Details
.sql
,.prepare
and.exec
calls.This relies on the work done in #17858 and #17886